Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added nsxt_license Resource #423

Merged
merged 5 commits into from
Oct 14, 2020
Merged

Added nsxt_license Resource #423

merged 5 commits into from
Oct 14, 2020

Conversation

ColemanJohnston
Copy link

Added a resource to allow terraform users to apply an NSX-T license.

@ghost ghost added size/l Relative Sizing: large documentation Documentation labels Aug 13, 2020

The following arguments are supported:

* `license_key` - (Required) NSX-T license key.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Coleman, looks good, just one nit - if import is supported, it should be mentioned here in the docs.
I also think that resources that are dependent on this license will fail to create/delete together with the licence, unless an explicit depends_on is specified in resource config. I don't think we can fix this easily, but worth mentioning this here in the doc.

return &schema.Resource{
Create: resourceNsxtLicenseCreate,
Read: resourceNsxtLicenseRead,
Update: resourceNsxtLicenseCreate,
Copy link
Collaborator

@annakhm annakhm Aug 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove the Update here and thus forbid the update

@@ -112,6 +112,10 @@ func getTestCertificateName(isClient bool) string {
return os.Getenv("NSXT_TEST_CERTIFICATE_NAME")
}

func getTestLicenseKey() string {
return os.Getenv("NSXT_LICENSE_KEY")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets call it NSXT_TEST_LICENSE_KEY to align with other test env settings. Thanks!

CHANGELOG.md Outdated
@@ -1,4 +1,8 @@
## 2.2.0 (Unreleased)

FEATURES:
* **New Resource**: `nsxt_license`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not accurate any more

nsxt/provider.go Outdated
}

func configureLicenses(d *schema.ResourceData, clients *nsxtClients) error {
for _, lic_key := range d.Get("license_keys").([]interface{}) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we only do this is d.HasChange("license_keys").
Also shouldn't we handle license removal as well?
d.GetChange can help determine which ones were added and which ones removed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems as if provider attributes are not persisted, and d.HasChange returns true even when there has not been a change in configuration. d.GetChange always produces an empty list and the current list instead of the previous configuration in this case. It may make sense for License to be a resource if removal is necessary. Should we keep license in the provider and not handle removal, or make it a resource and document the need for explicit depends_on?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.. Removing a license is probably a rare use case.. I'm ok with leaving this as is (it would be nice to leave a comment about this in the code and in docs).

Coleman Johnston added 4 commits October 12, 2020 13:27
Added a resource to allow terraform users to apply an NSX-T license.
Added license_keys argument to the provider block to
allow users to add nsx-t license keys there.
Added docs that explain the lack of
license key delete process.
@annakhm annakhm requested review from asarfaty and enhaocui October 13, 2020 16:44
@annakhm
Copy link
Collaborator

annakhm commented Oct 14, 2020

Hi Coleman, sorry to ask for yet another change on this..
Could you please add ConflictsWith: []string{"vmc_token"}` to the new provider attribute. This way we can protect the vmc use case from trying to configure the license, which will not work.

This change prevents vmc token from being used with license.
@annakhm annakhm merged commit 0ad7c8b into vmware:master Oct 14, 2020
@ColemanJohnston ColemanJohnston deleted the resource_license branch October 14, 2020 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation size/l Relative Sizing: large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants